chore: Add deps/deps-dev to commit lint rules#330
chore: Add deps/deps-dev to commit lint rules#330ayushiahjolia wants to merge 2 commits intomainfrom
Conversation
ayushiahjolia
changed the title
ayushiahjolia
changed the title
ayushiahjolia
changed the title
There was a problem hiding this comment.
I think we could remove lintcommit.js check entirely. It actually just checks the PR (not the commits in the PR). Currently this adds considerable friction and is only relevant to the PR. Also, using js in a python repo is adding churn to PR (in fact, a lot of the failing PR titles are coming from dependabot trying to update nodejs. which is only there to service this)
For a clean commit history, the commit msgs are more important than the PR itself.
I would be interested in maybe instead of this, to amend so it checks the commits that it follows these rules: https://github.com/aws/aws-durable-execution-sdk-python/blob/main/CONTRIBUTING.md#pull-request-title-and-commit-message-format
https://www.conventionalcommits.org/en/v1.0.0/
the most important:
- Subject line must be 50 characters or less
- Body text should wrap at 72 characters for good terminal display
- Use lowercase for type and scope
- Use imperative mood in subject ("add" not "added" or "adds")
- No period at the end of the subject line
ayushiahjolia
force-pushed
the
lint_update
branch
from
50b1d34 to
a1d8d99
Compare
ayushiahjolia
force-pushed
the
lint_update
branch
from
31b99d8 to
88b74e0
Compare
ayushiahjolia
force-pushed
the
lint_update
branch
from
1646cd7 to
6a7d69c
Compare
| if len(lines) > 1 and lines[1].strip() != "": | ||
| warnings.append("missing blank line between subject and body") | ||
|
|
||
| if len(lines) > 2: |
There was a problem hiding this comment.
body warning check is skipped entirely if subject + body with no blank line (len(lines) > 1)
when there's no
blank line, the body lines are at index 1+ instead of 2+, so the body length
check uses lines[2:] and misses line index 1.
| import sys | ||
| import unittest | ||
|
|
||
| sys.path.insert(0, os.path.dirname(os.path.dirname(__file__))) |
There was a problem hiding this comment.
in python the dir name should prob be tests, the dunder means something else in python.
|
|
||
| sys.path.insert(0, os.path.dirname(os.path.dirname(__file__))) | ||
|
|
||
| from lintcommit import validate_message, validate_subject |
There was a problem hiding this comment.
you shouldn't need to do this if you import from ops.lintcommit import
| return (error, warnings) | ||
|
|
||
|
|
||
| def _format_error(title: str, reason: str) -> str: |
There was a problem hiding this comment.
what calls this? is this dead code?
| if subject.endswith("."): | ||
| return "subject must not end with a period" | ||
|
|
||
| return None |
There was a problem hiding this comment.
could we enforce lowercase for the entire subject line, pls? strictly speaking not required by conventional commits, but nonetheless, all the examples are like that and it'd be nice to be consistent.
2 participants
Issue #, if available: N/A
Description of changes: Version bump PRs are failing lint checks because current lint config only allows sdk and examples types. Adding 2 more to fix lint failures - #326
Testing:
And, updated tests.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.